Implement auth #186
Implement auth #186
Conversation
setrofim
force-pushed
the
setrofim-auth
branch
from
d8054cc
to
e7f8872
Compare
| WORKDIR /opt/keycloak | ||
| # note: for development set up early; use proper certification in production. | ||
| RUN keytool -genkeypair -storepass password -storetype PKCS12 -keyalg RSA -keysize 2048 \ | ||
| -dname "CN=server" -alias server -ext "SAN:c=DNS:localhost,IP:127.0.0.1" \ |
There was a problem hiding this comment.
should the server IP on line 6 be hard coded or read from yaml file?
There was a problem hiding this comment.
nah, this isn't really used. This sets up keys used for the HTTPS cert, but we're enabling and using HTTP (specifically to avoid having to deal with certs in the test deployment). There isn't really a way to disable HTTPS (that I've seen), so the keys need to generated even if they're not used.
| @@ -11,6 +11,7 @@ VTS_PORT ?= 50051 | |||
| PROVISIONING_PORT ?= 8888 | |||
| VERIFICATION_PORT ?= 8080 | |||
| MANAGEMENT_PORT ?= 8088 | |||
| KEYCLOAK_PORT ?= 11111 | |||
There was a problem hiding this comment.
Just a thought, this could be 8880 ?
There was a problem hiding this comment.
I specifically chose something different, since this isn't really part of Veraison -- this is an external service Veraison depends on (besides, its already difficult enough to remember the correct port to use for DEBUG_PORT, as they're all just 8's and 0's in various permutations).
| @@ -73,8 +74,19 @@ func main() { | |||
| provisioner := provisioner.New(vtsClient) | |||
|
|
|||
| log.Infow("initializing provisioning API service", "address", cfg.ListenAddr) | |||
| authorizer, err := auth.NewAuthorizer(subs["auth"], log.Named("auth")) | |||
There was a problem hiding this comment.
Presumably if the Line 41 auth subs is missing the NewAuthroizer will default to passthrough ?
There was a problem hiding this comment.
setrofim
force-pushed
the
setrofim-auth
branch
from
e7f8872
to
91695a7
Compare
|
Comments addressed. |
setrofim
force-pushed
the
setrofim-auth
branch
2 times, most recently
from
7330858
to
5949699
Compare
Signed-off-by: Sergei Trofimov <[email protected]>
setrofim
force-pushed
the
setrofim-auth
branch
from
5949699
to
d9f2baa
Compare
auth/basic.go
Outdated
| c.Writer.Header().Set("WWW-Authenticate", "Basic realm=veraison") | ||
| c.AbortWithStatus(http.StatusUnauthorized) |
There was a problem hiding this comment.
(just a suggestion)
To make this a bit more user friendly (i.e., to let the user distinguish among the different 401 root causes) we could return a "problem details" payload like we do elsewhere.
(this would apply to all other 401 returns in this scope.)
There was a problem hiding this comment.
Good point. Will add that.
Incidentally, we really need to think about how we're organising our code. There is enough commonality between the different REST API (verification, provisioning, etc) implementations, such as the ReportProblem, that it would make sense to have a common place for the code, rather than duplicationg it for each API.
There was a problem hiding this comment.
Good point. Will add that.
awesome
Incidentally, we really need to think about how we're organising our code. There is enough commonality between the different REST API (verification, provisioning, etc) implementations, such as the
ReportProblem, that it would make sense to have a common place for the code, rather than duplicationg it for each API.
➕1
| } | ||
|
|
||
| o.config = ginkeycloak.KeycloakConfig{ | ||
| Url: fmt.Sprintf("http://%s:%s", cfg.Host, cfg.Port), |
There was a problem hiding this comment.
It is quite security critical that the authoriser is authenticated.
(note to self: we need an issue for moving all the service endpoints to HTTPS.)
There was a problem hiding this comment.
(note to self: we need an issue for moving all the service endpoints to HTTPS.)
Added #191
|
|
||
| // note: this mapper function will only be called once the JWT had | ||
| // alreadybeen verified by ginkeycloak, so extracting claims without | ||
| // verification here is, in fact, safe. |
There was a problem hiding this comment.
paranoid mode on: have we checked that keycloak refuses JWTs with none algorithm?
There was a problem hiding this comment.
haven't tried that, as the only way to easlily get a JWT than can be used for this is from keycloak it self. We can test it out at some point, but I would argue that is a security issue with keycloak.
I.e. it is somehting that needs to be tested and secured for a particular deployment, not withithin veraison code.
There was a problem hiding this comment.
[...] but I would argue that is a security issue with keycloak.
yes
BTW, timely (but yet to be answered) question: keycloak/keycloak#22744
There was a problem hiding this comment.
oh, yeah. Thanks for finding that. Upvoted and subscribed.
setrofim
force-pushed
the
setrofim-auth
branch
from
d9f2baa
to
792e166
Compare
Implement API authentication and authorization. - Define IAuthorizer interface that can be used to obtain a gin middleware handler that performs authorisation. - Add a mechanism to obtain an IAuthorizer for a particular role base on Veraison configuration. - Implement "passthrough" authorizer that duplicates existing behavior (no auth). - Implement "basic" authorizer that does not rely on an external authorization server. - Implement "keycloak" authorizer that uses Keycloak service for authentication. - Update provisioning service to authorize based on "provisioner" role. - Update management service to authorize based on "manager" role. - Add the previously missing README for the management service. Signed-off-by: Sergei Trofimov <[email protected]>
- Update the deployment diagram to match current deployment. This includes diagramming the management service (was neglected when the service was added to the deployment). - Update REAME text around the diagram to match current deployment. - Add *.bkp to .gitignore. draw.io generates .bkp files when saving an updated diagram. Signed-off-by: Sergei Trofimov <[email protected]>
setrofim
force-pushed
the
setrofim-auth
branch
from
792e166
to
96f89d7
Compare
Implement API authentication and authorization.
middleware handler that performs authorisation.
Veraison configuration.
(no auth).
authorization server.
authentication.
note: Once the corresponding client-side support for auth has been added, there will be additional changes to update the
colci/pocliinside the docker deployment.